Emit more specific security state change events #92
Conversation
Needed for brave/browser-laptop#5524 Auditors: @bridiver @darkdh
| break; | ||
| default: break; | ||
| if (explanations.ran_insecure_content) { | ||
| Emit("security-style-changed", "active-mixed-content"); |
There was a problem hiding this comment.
don't we already have something for active and passive mixed content? @darkdh ?
There was a problem hiding this comment.
I think it's fine to consolidate here, just don't want to duplicate
There was a problem hiding this comment.
@bridiver, we have did-display-insecure-content for passive mixed content and did-run-insecure-content for active one. https://github.com/brave/electron/blob/master/atom/renderer/content_settings_client.cc#L408-L424
There was a problem hiding this comment.
@diracdeltas - I think we might be mixing the meanings of these things a little bit. active and passive mixed content are metadata about the current security style. The explanations also have a summary and description that would be useful for the ui so maybe this should emit more fields? Maybe the security style (without passive or active), the summary and description (from explanations.secure_explanations, unauthenticated_explanations or broken_explanations as appropriate to the security style) and then two bools for ran and displayed insecure content? It looks like there can actually be more than one explanation for the current state so maybe we should use a converter to map the explanations to an array of hashes [{summary: "..", desc: "..."}, ...]. No reason to worry about a fromV8 converter because we never send them back
There was a problem hiding this comment.
@darkdh I think we can also keep the existing event because it provides the url and this doesn't appear to
There was a problem hiding this comment.
actually forget about the two bools, let's just convert the SecurityStyleExplanations to a javascript object with whatever fields we want to hold everything
There was a problem hiding this comment.
so it turns out that this change does cause problems with @darkdh's handling of active mixed content. however, it's still needed for passive mixed content (which has a SecurityStyle of 'insecure' but should be treated by Brave front-end as secure), so i'm pushing a commit now to just fix that.
There was a problem hiding this comment.
i agree this should emit all the information available in some kind of object; will do after this release
| } else if (explanations.displayed_insecure_content) { | ||
| Emit("security-style-changed", "passive-mixed-content"); | ||
| } else { | ||
| Emit("security-style-changed", security_style); |
There was a problem hiding this comment.
++ on name change and converter
Requires brave/muon#92 Fix #5524 Auditors: @darkdh @bridiver Test Plan: covered by automated test
| default: break; | ||
| if (explanations.displayed_insecure_content && | ||
| security_style == content::SECURITY_STYLE_UNAUTHENTICATED) { | ||
| Emit("security-style-changed", "passive-mixed-content"); |
There was a problem hiding this comment.
I really think we're passing through the wrong info here. passive-mixed-content is additional information about the security style and it should be a 3rd param imo because it's somewhat independent of the security style
There was a problem hiding this comment.
@diracdeltas your last comment didn't show up on github and I just now saw the email. I'll merge
Needed for brave/browser-laptop#5524
Auditors: @bridiver @darkdh